Removed DBToken. Made the components uses Managed Identity #3584
Conversation
anshulvermapatel
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
jaitaiwan,
hlipsig and
tiguelu
as code owners
|
Please rebase pull request. |
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195
branch
from
4156bcd
to
f89ebdd
Compare
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195
branch
from
f89ebdd
to
4bd69b9
Compare
There was a problem hiding this comment.
Also need to remove:
rm docs/dbtoken-service.md- The load balancing rule to allow gateway to talk to the dbtoken service from
pkg/deploy(should be in templates)
We probably don't need to peer the gateway and RP vnets anymore. We might be able to remove that peering as well. Which would be good for overall security posture.
|
/azp run ci,e2e |
|
Pull request contains merge conflicts. |
|
Please rebase pull request. |
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195
branch
from
4bd69b9
to
20a5400
Compare
| logrusEntry := log.WithField("component", "database") | ||
| dbAuthorizer, err := database.NewMasterKeyAuthorizer(ctx, logrusEntry, msiToken, clientOptions, _env.SubscriptionID(), _env.ResourceGroup(), dbAccountName) | ||
| scope := []string{fmt.Sprintf("https://%s.%s", dbAccountName, _env.Environment().CosmosDBDNSSuffixScope)} |
There was a problem hiding this comment.
nit: I've seen this same code 3 times in this PR so far, would it be worth extracting into something well named?
There was a problem hiding this comment.
Well, this was the way it was previously, I think lets address this in a separate PR
| BackendAddressPool: &mgmtnetwork.SubResource{ | ||
| ID: to.StringPtr("[resourceId('Microsoft.Network/loadBalancers/backendAddressPools', 'rp-lb-internal', 'rp-backend')]"), | ||
| }, |
There was a problem hiding this comment.
This is the only removal that concerns me here and generally with the LB, it could just be an odd naming choice, but to be sure, this isn't removing the LB config for anything related to the RP Backend service in an impactful way, right?
There was a problem hiding this comment.
rp-lb-internal is currently being used for gateway to request dbtoken service in RP, apart from that in my knowledge it is not used for anything else. Now since dbtoken service is being removed, there is no use of this LB
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195
branch
from
b3b7303
to
555aec7
Compare
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Pretty sure e2e failed on the cert re-running. /azp run e2e |
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hlipsig
dismissed
jaitaiwan’s stale review
Outdated and changes have been made since,
Ben I'm going to make a follow on JIra and PRs for these because you're right but I don't think they should block the main body of work. |
hlipsig
dismissed
bennerv’s stale review
Creating follow on for remaining actions.
Which issue this PR addresses:
As part of the ARO-5512 , we needed to disable local auth for cosmosdb. In order to disable local authentication, this PR is to make our components rely on managed identities/entra id rather than keys for authentication/authorizing cosmosdb.
This is currently a draft PR as I will be testing it in the INT env first before going forward, however, reviews are appreciated.
I have manually changed some generated code here. That was because the PR I created to have the generated code changed is not merged yet. Once that is merged, I will generate the code from the go-cosmosdb repo itself just like how we usually do.Fixes
ARO-7195
ARO-7314
Test plan for issue:
There are multiple ways this can be tested.